Skip to content

fix(claim): do not report accept when decision persist fails - #33

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f004-claim-persist-error
Open

fix(claim): do not report accept when decision persist fails#33
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f004-claim-persist-error

Conversation

@SebTardif

@SebTardif SebTardif commented Aug 29, 2026

Copy link
Copy Markdown

What Problem This Solves

Fixes an issue where a reviewer pressing Accept on a clawtributor claim would be told the claim was accepted even when saving the decision to the database failed. Discord could add the clawtributors role, then the bot still DMed the claimant, posted the announcement, and replied "Claim accepted" while the stored claim stayed pending.

That false success is worse than a failed Accept: staff think the review is finished, the claimant is told they are a clawtributor, and a later /claim still looks pending.

Why This Change Was Made

Accept now waits for recordClaimDecision to finish. If the write fails, the review button replies with an error and does not DM, announce, or mark the review message accepted. The previous success copy is sent only after the decision is saved. Reject is unchanged.

User Impact

Reviewers get an accurate private reply when Accept cannot save the decision. A database failure no longer looks like a completed clawtributor grant. Claimants are not told they were accepted while the claim record is still pending.

Evidence

Live bun run of /tmp/proof-hermit-F004.ts against upstream src/server/claimServer.ts and this branch. The script calls the production Accept button (claimReviewComponents[0].run) after Discord's role PUT returns 204. One run makes recordClaimDecision throw database unavailable. The other run lets the write finish.

$ bash /tmp/proof-hermit-F004.sh
===== BEFORE (upstream accept swallows persist errors) =====
PERSIST REJECT
{
  "reply": "Claim accepted. <@user-1> has been given the role.",
  "dmCount": 1,
  "announcementCount": 1,
  "patchCount": 1
}
PERSIST OK
{
  "reply": "Claim accepted. <@user-1> has been given the role.",
  "dmCount": 1,
  "announcementCount": 1,
  "patchCount": 1
}
===== AFTER (patched accept surfaces persist errors) =====
PERSIST REJECT
{
  "reply": "Could not record claim. The clawtributors role was added, but the claim decision could not be saved. Ask a moderator to retry or update the claim record.",
  "dmCount": 0,
  "announcementCount": 0,
  "patchCount": 0
}
PERSIST OK
{
  "reply": "Claim accepted. <@user-1> has been given the role.",
  "dmCount": 1,
  "announcementCount": 1,
  "patchCount": 1
}

Before this patch a failed decision write still printed Claim accepted, sent the DM, announced the grant, and patched the review message. After the patch a failed write prints Could not record claim and skips those success side effects. A successful write still prints Claim accepted.

Related: the swallow landed in #10 (2b970350, 2026-05-12). Same false-success class as #26 (automod-bypass role changes) and #27 (inactivity-warn addMember).

Real behavior proof

  • Behavior or issue addressed: Review-channel Accept reported Claim accepted, DMed the claimant, and announced the clawtributors grant even when saving the accepted decision failed, so the stored claim stayed pending.

  • Real environment tested: macOS 26.6.1 Darwin 25.6.0 arm64, Node v26.7.0, bun 1.3.14, patched checkout /tmp/oc-pr-hermit-F004 on fix/f004-claim-persist-error (base 72b5de1).

  • Exact steps or command run after this patch:

    bash /tmp/proof-hermit-F004.sh

  • Evidence after fix: terminal output from the patched Accept handler:

    $ bash /tmp/proof-hermit-F004.sh
AFTER persist reject
{
  "reply": "Could not record claim. The clawtributors role was added, but the claim decision could not be saved. Ask a moderator to retry or update the claim record.",
  "dmCount": 0,
  "announcementCount": 0,
  "patchCount": 0
}
AFTER persist ok
{
  "reply": "Claim accepted. <@user-1> has been given the role.",
  "dmCount": 1,
  "announcementCount": 1,
  "patchCount": 1
}

  • Observed result after fix: A failed decision write now replies Could not record claim and does not DM, announce, or patch the review message. A successful write still replies Claim accepted and still DMs, announces, and patches.

  • What was not tested: A live Discord review-channel click against production D1. Discord role PUT was stubbed to 204 so the persist path is the only failure.

Summary

Accept no longer treats a swallowed recordClaimDecision error as a completed grant. Persist is awaited on the success path. Persist failure replies with an error.

@SebTardif
fix(claim): do not report accept when decision persist fails
38a96ea 
Accept waited for the clawtributors role PUT, then recorded the
decision with a log-only catch. A database failure still DMed the
claimant, announced the grant, and replied Claim accepted while the
stored claim stayed pending.

Await recordClaimDecision on the success path. On persist failure,
reply with an error and skip the DM, announcement, and accepted reply.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 5:06 PM ET / 21:06 UTC.

ClawSweeper review

What this changes

The PR makes the Discord claim-review Accept button show a recovery message and stop success-side effects when saving an accepted claim decision throws, with focused regression coverage.

Merge readiness

⛔ Blocked until real behavior proof from a real setup is added - 3 items remain

Keep open: the branch correctly prevents success notifications after a failed claim-decision write, and current main still has the log-only failure path. The supplied proof and added test replace the actual decision writer, so real persistence-failure recovery remains unproven before merge.

Priority: P2
Reviewed head: 38a96ea9d456155ea809ad88330a85fe55a910bc

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The implementation and focused tests are solid, but merge readiness is capped by mock-only persistence proof.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The changed production owner is the claim-review Accept button, and the supplied terminal trace reaches its failure and success branches; however, both that trace and the added test stub the Discord role request and replace the real decision writer, so they do not prove observed recovery through actual persistence. Add redacted production-path evidence for a real write failure and a successful write, then update the PR body for re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed production owner is the claim-review Accept button, and the supplied terminal trace reaches its failure and success branches; however, both that trace and the added test stub the Discord role request and replace the real decision writer, so they do not prove observed recovery through actual persistence. Add redacted production-path evidence for a real write failure and a successful write, then update the PR body for re-review.
Evidence reviewed 5 items Current main still needs this fix: The comparison with fetched current main shows main retains the previous log-only catch and does not contain this PR’s recovery reply or early return.
Introduced handler behavior: The PR catches a rejected decision write, replies with Carbon components, and returns before the claimant DM, announcement, review-message patch, and accepted reply.
Regression coverage: The added test asserts that a simulated rejected decision write produces one recovery reply and no DM, announcement, or message patch; it also checks the success path.
Findings None None.
Security None None.

How this fits together

Hermit’s Discord claim-review flow grants the clawtributors role, records the accepted decision in persistent storage, then notifies the claimant and updates the review message. This change adds a failure branch between persistence and those downstream notifications.

flowchart TD
A[Reviewer clicks Accept] --> B[Grant Discord role]
B --> C[Save accepted claim decision]
C -->|Saved| D[Notify claimant and update review]
C -->|Write fails| E[Show moderator recovery message]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The changed production owner is the claim-review Accept button, and the supplied terminal trace reaches its failure and success branches; however, both that trace and the added test stub the Discord role request and replace the real decision writer, so they do not prove observed recovery through actual persistence. Add redacted production-path evidence for a real write failure and a successful write, then update the PR body for re-review.
  • Resolve merge risk (P1) - The available trace and regression test replace the real decision writer, leaving recovery from an actual persistence failure unverified.
  • Complete next step (P2) - Contributor-supplied real behavior proof, rather than an automated code repair, is the remaining merge blocker.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +16, tests +126 One focused handler change is accompanied by two explicit failure/success scenarios.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Land this narrow ordering fix after redacted proof from a real claim-review path or approved production-path harness shows both an actual write failure and successful completion.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Land this narrow ordering fix after redacted proof from a real claim-review path or approved production-path harness shows both an actual write failure and successful completion.

Do we have a high-confidence way to reproduce the issue?

Yes—current main still logs a rejected decision write and continues to the success side-effects; forcing that writer to reject is a high-confidence focused path, although this read-only review did not execute it.

Is this the best way to solve the issue?

Yes—the branch’s persistence gate is the narrowest maintainable way to prevent a failed stored decision from being reported as accepted; the remaining gap is production-path proof rather than solution design.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 38f929b69cba.

Labels

Label justifications:

  • P2: A failed persistence operation can falsely complete a staff claim-review workflow, but the affected surface is limited to claim acceptance.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed production owner is the claim-review Accept button, and the supplied terminal trace reaches its failure and success branches; however, both that trace and the added test stub the Discord role request and replace the real decision writer, so they do not prove observed recovery through actual persistence. Add redacted production-path evidence for a real write failure and a successful write, then update the PR body for re-review.

Evidence

What I checked:

  • Current main still needs this fix: The comparison with fetched current main shows main retains the previous log-only catch and does not contain this PR’s recovery reply or early return. (src/server/claimServer.ts:204, 38f929b69cba)
  • Introduced handler behavior: The PR catches a rejected decision write, replies with Carbon components, and returns before the claimant DM, announcement, review-message patch, and accepted reply. (src/server/claimServer.ts:203, 38a96ea9d456)
  • Regression coverage: The added test asserts that a simulated rejected decision write produces one recovery reply and no DM, announcement, or message patch; it also checks the success path. (tests/claimAcceptPersist.test.ts:71, 38a96ea9d456)
  • Proof remains mock-only: The PR body’s terminal trace invokes the production button but stubs the Discord role request and replaces recordClaimDecision; it demonstrates control flow, not recovery through the real persistence owner.
  • Claim-persistence history: The merged claim-deduplication feature commit is available locally and records fuller-stack-dev as author; it is useful routing context for the persistence path, though the partial clone could not provide an exact historical source-hunk comparison. (src/data/claimRequests.ts:100, 2b9703501567)

Likely related people:

  • fuller-stack-dev: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add redacted evidence from a real claim-review path or approved production-path harness using the actual decision writer, showing failed-write recovery and successful completion.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (13 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-30T17:04:14.694Z sha 38a96ea :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-30T21:30:15.027Z sha 38a96ea :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-31T04:00:28.320Z sha 38a96ea :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-31T08:08:12.015Z sha 38a96ea :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-31T12:08:22.395Z sha 38a96ea :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-01T04:57:58.641Z sha 38a96ea :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-01T09:00:54.960Z sha 38a96ea :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-01T12:03:20.297Z sha 38a96ea :: needs real behavior proof before merge. :: none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@SebTardif